roachtest: run INSPECT as background job and poll for completion #155653
Conversation
|
This change is |
spilchen
force-pushed
the
gh-155610/251017/1508/inspect-long-run/pr-ready
branch
from
3a5fe19 to
a2a23a2
Compare
There was a problem hiding this comment.
@bghal reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/cmd/roachtest/tests/inspect_throughput.go line 304 at r1 (raw file):
) (jobID int64) { // Set a short statement timeout to force INSPECT to run as a background job. if _, err := db.Exec("SET statement_timeout = '5s'"); err != nil {
Nit: Seems like the require package can be used in roachtests.
pkg/cmd/roachtest/tests/inspect_throughput.go line 311 at r1 (raw file):
// Reset statement timeout. if _, resetErr := db.Exec("RESET statement_timeout"); resetErr != nil {
Does this need to break up the error checking? Could it be deferred?
pkg/cmd/roachtest/tests/inspect_throughput.go line 316 at r1 (raw file):
// We expect a statement timeout error, which is normal. if err != nil && !strings.Contains(err.Error(), "statement timeout") {
Nit: Similarly require.ErrorContains would be nice.
pkg/cmd/roachtest/tests/inspect_throughput.go line 361 at r1 (raw file):
// Check if job is complete. if status == "succeeded" {
Nit: Perhaps a switch on jobs.StatusMessage?
Long-running INSPECT statements (e.g., >3h) have previously failed due to connection resets, despite no server crash. This likely stems from a missed keep-alive or a client timeout, though the exact cause is unclear. This change updates the roachtest to: - Run the INSPECT statement as a background job using a short statement_timeout (5s). - Poll the job table for job completion instead of waiting synchronously. - Report progress at 10% intervals to improve visibility without overwhelming the logs. Fixes cockroachdb#155610 Release note: none Epic: none
spilchen
force-pushed
the
gh-155610/251017/1508/inspect-long-run/pr-ready
branch
from
a2a23a2 to
f8b539b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/inspect_throughput.go line 304 at r1 (raw file):
Previously, bghal wrote…
Nit: Seems like the
requirepackage can be used in roachtests.
Good idea.
pkg/cmd/roachtest/tests/inspect_throughput.go line 311 at r1 (raw file):
Previously, bghal wrote…
Does this need to break up the error checking? Could it be deferred?
I'll reset this in a defer
pkg/cmd/roachtest/tests/inspect_throughput.go line 361 at r1 (raw file):
Previously, bghal wrote…
Nit: Perhaps a switch on
jobs.StatusMessage?
Good idea.
|
TFTR! bors r+ |
155411: restore: prevent scatter of non-empty ranges in online restore r=jeffswenson a=kev-cao In conventional restore, we prevent the scattering of non-empty ranges by passing in a max size for the `AdminScatter` request. In online restore, while we expect to almost always be scattering empty ranges, due to the fact that the link phase can retry, it is possible to scatter ranges that contain external SSTs. This breaks our golden rule of only scattering empty ranges. This commit teaches online restore to only ever scatter empty ranges. Fixes: #144599 Release note: None 155653: roachtest: run INSPECT as background job and poll for completion r=spilchen a=spilchen Long-running INSPECT statements (e.g., >3h) have previously failed due to connection resets, despite no server crash. This likely stems from a missed keep-alive or a client timeout, though the exact cause is unclear. This change updates the roachtest to: - Run the INSPECT statement as a background job using a short statement_timeout (5s). - Poll the job table for job completion instead of waiting synchronously. - Report progress at 10% intervals to improve visibility without overwhelming the logs. Fixes #155610 Release note: none Epic: none 155657: sql: admins now have full access to all external connections r=rafiss a=kev-cao Previously, if a non-admin user were to create an external connection, that connection would not show up in the output of `SHOW EXTERNAL CONNECTIONS` that was run by an admin. This patch fixes this bug so that admins can now see/use all external connections. Fixes: #146773 Release note: Admin users now have full access to all external connections. Co-authored-by: Kevin Cao <[email protected]> Co-authored-by: Matt Spilchen <[email protected]>
|
Build failed (retrying...): |
3 participants
Long-running INSPECT statements (e.g., >3h) have previously failed due to connection resets, despite no server crash. This likely stems from a missed keep-alive or a client timeout, though the exact cause is unclear.
This change updates the roachtest to:
Fixes #155610
Release note: none
Epic: none